Skip to content

api: Time out long-poll requests, as the server recommends - #2396

Open
chrisbobbe wants to merge 9 commits into
zulip:mainfrom
chrisbobbe:pr-time-out-poll-requests
Open

api: Time out long-poll requests, as the server recommends#2396
chrisbobbe wants to merge 9 commits into
zulip:mainfrom
chrisbobbe:pr-time-out-poll-requests

Conversation

@chrisbobbe

Copy link
Copy Markdown
Collaborator

Fixes #514.

Upcoming commits will rework how the message is chosen and throw the
same way from a second call site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chrisbobbe
chrisbobbe force-pushed the pr-time-out-poll-requests branch 2 times, most recently from 9dfb6cf to 604be31 Compare July 29, 2026 01:10
claude added 8 commits July 29, 2026 16:36
When the initial registerQueue attempt fails, we report the error to
the user unless it is a routine connection failure.  That decision had
no test coverage.  Add some now, ahead of reworking in the next commit
how the decision is made.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When deciding how to handle a NetworkException, callers matched on
whether its cause was a SocketException, in order to recognize the
routine connection failures that happen when the device is offline or
returns from sleep.  That ties the decision to the exception types of
dart:io.  A different HTTP client implementation, like the cronet_http
and cupertino_http clients considered in zulip#461, reports these conditions
with different types (both wrap errors in plain ClientException
subclasses); the app would quietly begin showing the user connection
errors whenever a routine failure persisted past a few poll retries,
as after waking from a long sleep.

So classify the cause once, in _throwNetworkException, into a new
NetworkExceptionKind, chosen together with the user-facing message in a
single switch; and have callers switch on the kind instead.

No change in behavior, as the tests added in the previous commit help
confirm.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For a connection failure, package:http does not hand us the bare
SocketException that FakeApiConnection simulates.  Its IOClient catches
that and rethrows a private _ClientSocketException, which extends
ClientException and also implements SocketException.  That is
deliberate, so as not to break callers that catch the latter.

The switch in _throwNetworkException handles the wrapped form with its
own arm, ahead of the plain-ClientException arm that would classify it
as `other`.  A plausible edit, simplifying the switch or reordering its
arms, would silently break that; then routine connection failures, like
when the device is offline, would be reported to the user once they
persisted past a few poll retries.  So add a stand-in for the wrapped
form, and a case pinning down how it is classified.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This is the timeout the server recommends for GET /events requests;
an upcoming commit will start using it.

The field has been present since Zulip 5.0 (feature level 74), well
below our minimum, so it needs no fallback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The poll loop will use this in the next commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
package:http's Abortable lets a request carry a trigger that aborts
it.  Teach FakeHttpClient the behavior IOClient has for that: if the
trigger fires before the response headers are delivered, the send
future throws a RequestAbortedException; if it fires while the body is
streaming, the same exception is injected into the body stream.

Also add a `bodyDelay` option to `prepare`, so a test can arrange for
a response's body to still be in flight at a chosen moment.

The next commit will use these to test a timeout on getEvents.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the caller passes `timeout` and the request does not complete
within it, including reading the response body, the request is aborted
outright, with package:http's Abortable: that tears down the
connection rather than leaking it.  The resulting
RequestAbortedException classifies as
NetworkExceptionKind.connectionFailed, the same as the other ways of
losing a connection.

The next commit will use this to time out long-poll requests (zulip#514).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes zulip#514.

On a degraded or switching network, the GET /events connection can die
without the socket erroring: packets stop arriving, but nothing tells
us so.  A long-poll is especially bad at noticing this, because silence
for up to a minute is exactly what a healthy long-poll looks like.  We
then wait for the OS to give up on the TCP connection, which can take
minutes, and meanwhile no events reach the app: no incoming messages,
and the local echo of a message the user just sent stays on screen
looking stuck.

The server anticipates this: it sends heartbeat events so that silence
is bounded, and event_queue_longpoll_timeout_seconds says how long to
wait before concluding the request or its response was lost.  The API
docs ask long-lived clients to respect that value, so that a later
change to the heartbeat interval does not break them.

So pass it as a timeout on each getEvents request, the same way the
web app does.  A timed-out request classifies as
NetworkExceptionKind.connectionFailed, so the poll loop retries it
quietly with backoff, the same as any other lost connection.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@chrisbobbe
chrisbobbe force-pushed the pr-time-out-poll-requests branch from 604be31 to 203d944 Compare July 29, 2026 20:44
@chrisbobbe
chrisbobbe marked this pull request as ready for review July 29, 2026 20:44
@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jul 29, 2026

@rajveermalviya rajveermalviya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chrisbobbe! LGTM, but I am not sure how best to test this. Comments below.

Also I notice that the commits currently are authored by Claude: Author: Claude <noreply@anthropic.com>, which doesn't seem correct and I think it is probably a caveat of Claude Code for Web?

Comment on lines +1037 to +1038
eventQueueLongpollTimeoutSeconds: 90,
elapse: const Duration(seconds: 90),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're explicitly specifying a value for eventQueueLongpollTimeoutSeconds, let's use a value that's lower than example default (in eg.initialSnapshot).

Comment thread lib/api/core.dart
Comment on lines +239 to 253
/// Run [body] with a trigger that fires after [timeout],
/// for constructing an [http.Abortable] request.
///
/// The timer is canceled when the returned future completes,
/// so a request that finishes in time leaves no timer behind.
Future<T> _withTimeout<T>(Duration timeout,
Future<T> Function(Future<void> abortTrigger) body) async {
final abortCompleter = Completer<void>();
final abortTimer = Timer(timeout, abortCompleter.complete);
try {
return await body(abortCompleter.future);
} finally {
abortTimer.cancel();
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's maybe move this helper below other HTTP-method methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer review PR ready for review by Zulip maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider overall timeout for long-poll requests

3 participants